Code Review: Corpus Reference Enrichment (#1976)

Overview

This is a large, well-scoped PR (12,318 additions, 46 files) that adds a deterministic corpus reference enrichment engine, a new CorpusReference model, a @corpus_analyzer_task decorator, cross-corpus law linking via AuthorityCorpusBootstrapper, agent tools, and a GraphQL query surface. The demo numbers are compelling (1,943 references resolved across 254 documents), and the 110-test suite is a strong foundation. Below are findings organized by severity.

🔴 Medium – GraphQL traversal may bypass document-level visibility

CorpusReferenceType exposes target_document, target_annotation, and target_corpus as traversable relay fields. CorpusReferenceService.for_corpus uses corpus-as-gate semantics — any user with corpus-READ can fetch all CorpusReference rows in that corpus, including rows whose target_document is a document with individual-level restrictions.

Whether this leaks depends on whether DocumentType enforces visibility per-field or only at the top-level documents query. If it's the latter, a query like:

corpusReferences(corpusId: "...") {
  edges { node { targetDocument { id title content } } }
}

could surface document content the user has no direct READ access to.

Action required: Confirm that DocumentType resolvers (and AnnotationType) enforce visibility when accessed as nested fields, OR add @visibility_required / custom field resolvers to CorpusReferenceType that null out related objects the requesting user cannot read. A test verifying this invariant would be valuable.

🔴 Medium – link_external_references and bootstrap fetch corpus without visibility check

# EnrichmentService.link_external_references
corpus = Corpus.objects.get(pk=corpus_id)  # no visibility check

# AuthorityCorpusBootstrapper.bootstrap
corpus = Corpus.objects.get(pk=corpus_id)  # no visibility check

These methods are currently called only from trusted pipeline/task code (agent tools inject corpus_id from context; Celery tasks inherit it from the Analysis row). The risk is contained today, but both methods accept caller-supplied integer PKs. If either is later called from a new user-context surface without a pre-check, it becomes a straightforward IDOR.

Per CLAUDE.md, the E001 system check only scans config/graphql/, so this won't trip CI — but the policy applies to MCP tools and agent tools too. Consider adding a visible_to_user guard at the top of both methods, or at minimum adding a comment documenting the caller-trust assumption so future callers know to pre-validate.

🟡 Bug risk – _terms() early return starves _TERM_MEANS_RE

def _terms(self, text: str) -> Iterator[Candidate]:
    ...
    for regex, kind in ((_TERM_PAREN_RE, "parenthetical"), (_TERM_MEANS_RE, "means")):
        for m in regex.finditer(text):
            if emitted >= C.MAX_DEFINED_TERMS: return  # stops outer loop too

If _TERM_PAREN_RE hits MAX_DEFINED_TERMS, the return exits the generator entirely — _TERM_MEANS_RE never runs. A document heavy with parenthetical-form defined terms (e.g., (the "Company") clauses) would exhaust the cap before any "X" means … patterns are tried. If the intent is a cap across both forms, this is correct but worth a comment. If the intent is to process both regex types up to the cap each, the logic needs rethinking (e.g., interleave or apply the cap per-regex-type).

🟡 Architecture – EnrichmentService / CorpusReferenceService location diverges from convention

CLAUDE.md: "Code with a user context must reach models through opencontractserver/<app>/services/"

Both services live in opencontractserver/enrichment/service.py (flat file, singular name) rather than opencontractserver/enrichment/services/ (directory). The E001 check won't flag this since enrichment/ is outside config/graphql/, but the pattern diverges and will create inconsistency as this module grows. Also, CorpusReferenceService manages a model from the annotations app — consider whether it belongs in opencontractserver/annotations/services/ or if enrichment/ owning its cross-app service is intentional.

🟡 Data integrity – REFERENCE_TYPE_CHOICES duplicated between two source-of-truth locations

annotations/models.py defines choices as module-level string literals:

REFERENCE_TYPE_CHOICES = [("LAW", "Law citation"), ("DOCUMENT", "Document reference"), ...]

enrichment/constants.py presumably defines REF_LAW = "LAW" etc. (used in extractor.py). These are the same strings in two places. If a new reference type is added, both must be updated in sync. The model should import and reference the constants from enrichment/constants.py rather than re-declaring them.

🟢 Minor – CorpusReferenceService.visible_to_user subquery may not scale

return CorpusReference.objects.filter(
    corpus__in=Corpus.objects.visible_to_user(user)
)

For users with hundreds of visible corpora, this generates a large IN (...) clause. The Django ORM will produce a subquery, which is fine for typical workloads, but as the platform scales to many corpora per user this is worth monitoring. A corpus__creator=user OR corpus__... pattern or an EXISTS subquery would scale more predictably.

🟢 Minor – corpus_analyzer_task decorator doesn't verify corpus-analysis ownership

Corpus.objects.filter(id=corpus_id).exists()  # existence only

The corpus_id is read from the Analysis row (trusted), so there's no IDOR risk. But the check provides weaker guarantees than a full Corpus.objects.visible_to_user(task_user).filter(id=corpus_id).exists() would. Consider tightening this for defensive depth — a task shouldn't be able to operate on a corpus the analysis-owner couldn't see when the analysis was created.

🟢 Minor – Defined-term choices defined after the model class

REFERENCE_TYPE_CHOICES and RESOLUTION_STATUS_CHOICES are defined at module level after the CorpusReference class definition. Django conventions (and readability) favor defining choices as class-level constants on the model or before the class they belong to. This also means the choices aren't accessible via CorpusReference.REFERENCE_TYPE_CHOICES — callers have to import from the module level.

✅ Strengths

• Permission architecture in the GraphQL layer is clean: resolve_corpus_references correctly delegates to CorpusReferenceService.for_corpus with no inline Tier-0 ORM — passes E001.
• Agent tools are approval-gated and write-gated correctly: apply has requires_approval=True, requires_write_permission=True; scan is read-only.
• Enrichment engine is DB-free and deterministic: extractor.py has no DB access — pure text processing with clear grammar boundaries. Easy to unit test and reason about.
• Idempotency is first-class: writer deduplicates on (document, label, span_start) for mentions and uses the unique constraint on CorpusReference(source_annotation, reference_type, canonical_key). Re-running enrichment on the same corpus is safe.
• Authority bootstrapper is data-driven: "adding a body of law is a bootstrap call, zero code" — the NYBCL end-to-end test pins this contract.
• CorpusDocumentService.get_corpus_documents used correctly in pipeline callers: EnrichmentService._load and _find_by_key/_find_by_title use corpus-as-gate semantics, which is the documented correct choice for pipeline-facing callers.
• 110 tests with good layering: separate test files per module (extractor/resolver/writer/authorities/linking/tools/analyzer-integration), including visibility tests in writer and tools.
• Changelog fragment is well-written: follows the fragment format, includes file paths and impact, covers all categories.
• demo/ is gitignore-clean: large import zips excluded, seed JSONs committed cleanly.

Test coverage gaps

1. GraphQL traversal leak (see 🔴 above): no test verifies that targetDocument / targetAnnotation on CorpusReferenceType respects document-level visibility when accessed through a readable corpus.
2. link_external_references with nonexistent corpus_id: Corpus.objects.get() will raise DoesNotExist uncaught — add an error-path test.
3. _terms() cap boundary: a test with exactly MAX_DEFINED_TERMS parenthetical terms would confirm the "means"-form is silently skipped and whether that's intentional.

Summary

The core architecture is solid and the permission handling in the GraphQL layer is correct. The two medium-severity items (traversal visibility and raw corpus lookups in pipeline methods) are worth addressing before merge. The _terms() early-return behavior is the most likely source of a subtle production bug. Everything else is cleanup / defensive hardening.

Review response (addressed in 17469c0, after merging main in 00e23a0)

🔴 GraphQL traversal visibility — Verified: nested FK traversal is framework-enforced. graphene-django 3.2.3's FK converter routes related-object resolution through _type.get_node(info, pk) → get_queryset, and both DocumentType.get_queryset and AnnotationType.get_queryset filter via BaseService.filter_visible_qs. Added CorpusReferenceTraversalVisibilityTests pinning the invariant: a reader of a public corpus sees the reference rows (corpus-as-gate) but gets null for a targetDocument they lack document-level READ on, while the owner resolves it.

🔴 Raw Corpus.objects.get in link_external_references / bootstrap — Fixed. EnrichmentService._load, link_external_references, and AuthorityCorpusBootstrapper.bootstrap now fetch via Corpus.objects.visible_to_user(user).get(pk=...), so invisible and nonexistent corpora raise the same Corpus.DoesNotExist (no existence oracle). Error-path tests added for both entry points.

🟡 _terms() starvation — Fixed, it was a real bug: the intent is a total per-document cap, but the per-regex loop could exhaust it on parentheticals before the means regex ever ran. Matches from both grammars are now merged in document order before capping (first N definition sites win regardless of form), documented in the docstring, and pinned by two boundary tests (means-form before the cap survives; after the cap is dropped).

🟡 Service location — Moved to opencontractserver/enrichment/services/ (corpus_reference_service.py + enrichment_service.py, re-exported from __init__), matching the repo-wide convention; all importers and mock-patch targets updated. CorpusReferenceService stays in enrichment/ deliberately: it is the read surface of the enrichment feature, and the annotations app has no knowledge of enrichment semantics — same pattern as other cross-app feature services. Bonus dedup: the local OC_SECTION_LABEL redeclaration now imports the canonical constant from constants/annotations.py.

🟡 Choices duplication — Fixed. REFERENCE_TYPE_CHOICES / RESOLUTION_STATUS_CHOICES discriminator values are now built from enrichment/constants.py (REF_*, STATUS_*); only the human-readable labels live in models.py. enrichment/constants.py is import-pure so no cycle, and makemigrations --check confirms the change is migration-neutral.

🟢 visible_to_user subquery scale — No change. The corpus__in=visible_to_user(...) shape matches the platform's current pattern; flagged for revisit if profiling shows it hot at many-corpora scale.

🟢 corpus_analyzer_task corpus check — Tightened. The existence check is now BaseService.filter_visible(Corpus, analysis.creator).filter(id=corpus_id).exists() — a task can no longer operate on a corpus the analysis owner couldn't see, with the same error message either way.

🟢 Choices defined after the model class — No change: the premise doesn't match the code. Both choice lists are defined immediately before class CorpusReference (annotations/models.py:1884-1894).

Test coverage gaps — All three covered: traversal-leak test, DoesNotExist error-path tests for link_external_references and bootstrap(corpus_id=...), and the _terms cap-boundary pair.

Verification: 95 tests green across the enrichment + analyzer suites (-n 0), pre-commit (incl. mypy) green, manage.py check green (E001 clean; the two remaining warnings are environment-state: unsynced analyzers in the fresh test container and a missing LlamaParse key).

Note for follow-up: makemigrations --check flags pending corpuses index-rename churn for CorpusVote (corpuses_co_session_idx → hashed name) that arrived with the merge from main — unrelated to this PR.

Code Review: Corpus Reference Enrichment (#1976)

This is a well-architected, substantial feature. The extract→resolve→write pipeline is clean and clearly separated, the idempotency design is solid, and the test coverage (110 tests) is strong. The changelog fragment and docs are exemplary. A few issues ranging from a potential bug to performance and style concerns below.

Potential Bug

_get_analysis uses id=C.ENRICHMENT_ANALYZER_ID where the ID value is a string slug

opencontractserver/enrichment/services/enrichment_service.py lines ~151–162:

analyzer, _ = Analyzer.objects.get_or_create(
    id=C.ENRICHMENT_ANALYZER_ID,          # "corpus-reference-enrichment"
    defaults={
        "task_name": C.ENRICHMENT_ANALYZER_TASK,
        ...
    },
)

ENRICHMENT_ANALYZER_ID = "corpus-reference-enrichment" is a string slug. If Analyzer.id is a BigAutoField integer PK (the Django default), this will raise a ValueError at runtime when the filter().first() miss triggers the get_or_create branch. The rest of the codebase creates analyzers using task_name= as the lookup key in auto_create_doc_analyzers. Recommend aligning:

analyzer, _ = Analyzer.objects.get_or_create(
    task_name=C.ENRICHMENT_ANALYZER_TASK,
    defaults={
        "description": C.ENRICHMENT_ANALYZER_TITLE,
        "creator_id": creator_id,
    },
)

Performance

N+1 queries in _resolutions

_sections_for(doc.id) issues one Annotation query per document. For a 100-document corpus this is 100 queries before writing begins. Batch with a single __in query before the loop:

doc_ids = [d.id for d in documents]
sections_by_doc: dict[int, list[_SectionAnno]] = defaultdict(list)
rows = Annotation.objects.filter(
    document_id__in=doc_ids, annotation_label__text=OC_SECTION_LABEL
).values_list("id", "document_id", "raw_text")
for pk, doc_id, txt in rows:
    sections_by_doc[doc_id].append(_SectionAnno(id=pk, raw_text=txt or ""))

Per-row saves in _link_external

_link_external loops over refs and calls ref.save(update_fields=[...]) individually. For a corpus with hundreds of law citations this is hundreds of round-trips. Use bulk_update on the batch (collect modified refs, then CorpusReference.objects.bulk_update(to_update, fields=[...])). Same for the mention.save(update_fields=["link_url", "modified"]) calls.

Code Style / Maintainability

_SectionAnno is a private symbol imported across module boundaries

enrichment_service.py imports the underscore-prefixed _SectionAnno from resolver.py. Either make it public (SectionAnno) or move the dataclass to a shared types.py within the enrichment package.

WriteResult uses None-then-__post_init__ instead of field(default_factory=list)

# current — requires type: ignore and __post_init__ boilerplate
annotation_ids: list[int] = None  # type: ignore[assignment]

# idiomatic
annotation_ids: list[int] = field(default_factory=list)

resolution_status default in CorpusReference model uses a raw string

resolution_status = models.CharField(..., default="RESOLVED")

Should reference enrichment_constants.STATUS_RESOLVED for the same single-source-of-truth reason the discriminator values do.

assert for runtime guard in production code

corpus_analysis_tasks.py:

assert analysis.creator_id is not None  # non-null FK; narrows for mypy

assert is elided when Python runs with -O. Use an explicit check or the cast/# type: narrow mypy idiom.

auto_create_doc_analyzers function name is now misleading

The function in analyzer/utils.py now handles corpus analyzers too. A small rename to auto_sync_analyzers (or at minimum updating the docstring/log messages to reflect both flavors) would reduce future confusion — the existing "auto-created from @doc_analyzer_task" default description still appears in the auto-create path.

Minor Issues

Empty authority_aliases edge case in _compile_law_re

If authority_aliases is ever passed as {}, alt becomes "" and the compiled regex has (?P<auth>) — an always-matching empty capture. Add a guard or short-circuit:

if not aliases:
    return re.compile(r"(?!)")  # never matches

Duplicate-count risk: _laws and _prefix_laws can match overlapping spans

For text like "Securities Act Section 4(a)(2)" the prefix regex matches, and for "Section 4(a)(2) of the Securities Act" the suffix regex matches — no overlap on a single occurrence. But the existing test test_prefix_and_suffix_forms_do_not_double_count only validates the suffix-vs-relative case, not suffix-vs-prefix. Consider adding a test that runs both grammars on text containing one law name to confirm only one candidate is produced.

GraphQL

The new CorpusReferenceType correctly omits AnnotatePermissionsForReadMixin (no guardian tables) and the CorpusReferenceService enforces corpus-level visibility scoping. No issues there.

The resolver does not go through request=info.context for the service call, which is the correct pattern here since CorpusReferenceService does its own visibility scoping rather than using the Tier-2 permission cache.

Summary

Code Review

Overview

This is a substantial, well-architected PR that adds corpus-level reference enrichment — law citations, document/exhibit references, internal section links, and defined terms — along with cross-corpus authority linking, a new @corpus_analyzer_task decorator, a GovernanceGraph GraphQL query, and 74 new tests across 7 files. The architecture is clean and follows project conventions well. Issues are ordered by severity below.

Bugs / Correctness

1. corpus_analyzer_task swallows Celery retries as permanent failures (opencontractserver/shared/decorators.py:445)

The bare except Exception block catches celery.exceptions.Retry before the re-raise, marks Analysis.status = FAILED, then re-raises. A retrying task permanently marks the analysis as failed before it actually retries. Compare with doc_analyzer_task at line 335, which explicitly catches Retry first and re-raises without touching the analysis:

# Fix — mirror doc_analyzer_task:
except Retry:
    raise
except Exception as e:
    analysis.status = JobStatus.FAILED.value
    ...
    raise

Performance

2. _link_external does O(N) individual save() calls (enrichment/services/enrichment_service.py ~line 210)

For a corpus with hundreds of law references this loops over individual saves for both CorpusReference and Annotation. CorpusReference.objects.bulk_update(updated_refs, [...]) and a single Annotation.objects.bulk_update(...) pass would collapse this to 2 queries. Not blocking for correctness, but the PR demo reports 1,943 resolved references.

3. _sections_for issues one DB query per document (enrichment/services/enrichment_service.py ~line 58)

Called in a loop over every document in the corpus. A single prefetch grouped by document_id at the top of _resolutions replaces N queries with 1:

sections_by_doc = defaultdict(list)
for pk, txt, doc_id in Annotation.objects.filter(
    document_id__in=[d.id for d in documents],
    annotation_label__text=OC_SECTION_LABEL,
).values_list('id', 'raw_text', 'document_id'):
    sections_by_doc[doc_id].append(_SectionAnno(id=pk, raw_text=txt or ''))

Code Quality

4. WriteResult dataclass uses the None-default + __post_init__ anti-pattern (enrichment/writer.py:34-40)

This requires # type: ignore[assignment] suppressions. The idiomatic form needs neither:

annotation_ids: list[int] = field(default_factory=list)
reference_ids: list[int] = field(default_factory=list)

5. auto_create_doc_analyzers name is stale (analyzer/utils.py)

The function now handles both doc- and corpus-scoped analyzers. Its internal comments and log messages were updated but the function name still says doc. Renaming to auto_create_analyzers would be consistent.

6. link_url uses raw integer PKs (enrichment/writer.py:93)

link_url = f"/corpus/{self.corpus.id}/document/{res.target_document_id}"

Verify the frontend router accepts raw integer PKs here rather than relay global IDs. From the demo HTML and CentralRouteManager this looks intentional, but worth confirming before the frontend integration lands.

Minor / Low Priority

7. CorpusReferenceService doesn't inherit BaseService (enrichment/services/corpus_reference_service.py)

Project convention (CLAUDE.md) is to extend opencontractserver.shared.services.base.BaseService. The current implementation is a plain class. Either extend it or add a note explaining the intentional deviation.

8. _get_analysis creates Analysis with status=COMPLETED from the start (enrichment/services/enrichment_service.py ~line 170)

The agent-tool path creates the Analysis already in COMPLETED state, bypassing the RUNNING transition. Correct design choice for that path, but a comment explaining the two code paths (framework-via-Celery vs. direct agent-tool call) would help future readers.

9. Import direction annotations/models.py → enrichment/constants.py: safe because constants.py is pure (no model imports). A brief comment noting this is deliberate would prevent future readers from flagging it as a potential cycle.

Test Coverage: Strong

74 new test methods cover: extractor grammar correctness, resolver dispatch, idempotent writes, cross-corpus linking, analyzer lifecycle (RUNNING/COMPLETED/FAILED), auto-sync, authority bootstrapper, governance graph visibility rules. A few notes:

• test_failure_inside_function_records_error verifies the FAILED transition — good. There is no test for the Retry case (issue 1 above). Adding one — patching EnrichmentService.apply to raise celery.exceptions.Retry and asserting the Analysis status is NOT set to FAILED — would lock in the fix once applied.
• test_prefix_and_suffix_forms_do_not_double_count is a nice edge-case catch for the overlapping regex grammars.
• Tests use real DB writes with TestCase rollback — correct for this level of integration.

Summary

Two items worth fixing before merge:

1. Retry exception handling in corpus_analyzer_task — two-line fix, prevents silent analysis corruption on transient failures.
2. _sections_for N+1 — not a correctness issue, but easy to batch and meaningful at scale.

Everything else is style or minor. The architecture is solid, the service layer is properly layered, and the permission model (corpus-as-gate for CorpusReferenceService, per-document READ check in GovernanceGraphService, consistent no-existence-oracle pattern) is correctly implemented throughout.

Review response (296423e)

1. Retry swallowed as permanent failure — Confirmed and fixed. Retry extends Exception, so the wrapper stamped Analysis.status=FAILED before the retry ran. corpus_analyzer_task now catches Retry first and re-raises untouched, mirroring doc_analyzer_task. Added the regression test you suggested: test_celery_retry_does_not_mark_analysis_failed pins RUNNING status and a clean error_message surviving a Retry.

6. link_url raw integer PKs — Verified, and it's worse than "worth confirming": no frontend route serves /corpus/{id}/document/{id} (App.tsx only routes slug shapes; that path falls into the * catch-all → 404). Both writers (EnrichmentWriter._get_or_create_mention and _link_external) now emit the canonical /d/{corpus.creator.slug}/{corpus.slug}/{document.slug} path via a new shared helper (opencontractserver/utils/frontend_paths.py::document_in_corpus_path, mirroring the frontend's buildCanonicalPath); cross-corpus law links point into the authority corpus, and missing slugs skip the link rather than writing a broken one. Both link-shape tests updated TDD-first (watched them fail on the old format).

2. _link_external O(N) saves — Fixed: refs and mentions accumulate and flush via two bulk_update calls, with modified stamped explicitly since bulk_update bypasses auto_now.

3. _sections_for N+1 — Fixed: replaced by _sections_by_doc, one grouped query per corpus.

4. WriteResult __post_init__ anti-pattern — Fixed with field(default_factory=list); the type: ignore suppressions are gone.

7. CorpusReferenceService doesn't extend BaseService — Fixed.

8. _get_analysis COMPLETED-from-the-start — Comment added documenting the two provenance paths (framework-via-Celery wrapper owns the lifecycle; the synchronous agent-tool path has no observable in-between state).

5. auto_create_doc_analyzers rename — Declining for now: three historical data migrations (analyzer/0009, 0012, 0013) import the function lazily at migrate time, so a rename breaks migrate on fresh databases unless we edit applied migrations or keep an alias (dead code per project rules). The name is stale but the churn/risk isn't worth it inside this PR; happy to do it as a follow-up that also touches the migrations deliberately.

9. Import-direction comment — Already present since 17469c06e: annotations/models.py:1881 carries "enrichment/constants.py is pure — no model imports — so this cannot cycle."

Verification: 188 tests green across the enrichment/analyzer/governance-graph/architecture suites (-n 0), pre-commit incl. mypy green.

Code Review — PR #1976: Corpus Reference Enrichment

This is a large, ambitious PR adding a well-architected new subsystem. The clean separation of concerns (extractor → resolver → writer → services → GraphQL) and thorough test coverage (110 tests) are commendable. Below are findings ranging from bugs to style nits.

Bugs

1. Missing malformed-ID guard in resolve_corpus_references (config/graphql/annotation_queries.py)

resolve_governance_graph protects itself with if not str(corpus_pk).isdigit(): return empty, but resolve_corpus_references does not:

def resolve_corpus_references(self, info, corpus_id, **kwargs) -> Any:
    pk = int(from_global_id(corpus_id)[1])   # ← raises ValueError on malformed ID

If from_global_id returns ("", "") (malformed relay ID), int("") throws ValueError and produces a 500 instead of a clean GraphQL error. Add the same .isdigit() guard here.

2. Analysis marked COMPLETED before writes succeed (enrichment_service.py:_get_analysis)

When apply() is invoked via the agent tool (not the Celery framework path), _get_analysis() persists the Analysis row with status=COMPLETED before writer.write() is called. If write() then raises (e.g., a DB error), the Analysis row will permanently show COMPLETED with no result. The transaction in write() only rolls back annotation/reference rows — the Analysis row was committed separately. Fix: create the Analysis with status=RUNNING, then update to COMPLETED after write() and _link_external() both succeed.

Performance

3. N+1 queries in EnrichmentWriter._get_or_create_mention() (writer.py:_get_or_create_mention)

One SELECT per resolution before each INSERT, inside the transaction.atomic() block. For a corpus with 1,943 references (as in the demo), this is ~1,943 individual queries. Consider bulk-fetching existing Annotation rows for the whole run (grouped by (document_id, annotation_label_id, json__start)) and checking in Python, reducing to O(1) queries.

4. N+1 path_records queries in _link_external() (enrichment_service.py:_link_external)

target_corpus_id = (
    target.path_records.filter(is_current=True, is_deleted=False)
    .values_list("corpus_id", flat=True)
    .first()
)

This fires one query per distinct resolved target document inside the loop. target_cache deduplicates the find_authority_target() calls, but each unique target document still issues an extra path_records query. Prefetch .path_records on the find_authority_target query results or batch this.

5. Single large transaction for the entire write (writer.py:write)

All N resolutions are wrapped in one transaction.atomic(). For a 1,943-reference corpus this holds a lock open for potentially tens of seconds, increasing contention. Consider batching in chunks of, say, 500 resolutions per transaction.

Code Quality

6. Import of private name (enrichment_service.py:L33)

from opencontractserver.enrichment.resolver import _SectionAnno

_SectionAnno is defined with a leading underscore (conventionally "private") but imported and used in a different module. Either rename it to SectionAnno (public) or move it to a shared types.py in the enrichment package.

7. assert used for input validation in the Celery task (corpus_analysis_tasks.py)

assert analysis.creator_id is not None  # non-null FK; narrows for mypy

AssertionError can be disabled with python -O. Use an explicit if not analysis.creator_id: raise ValueError(...) for validation that must run in production.

8. Misleading default on resolution_status (annotations/models.py:CorpusReference)

resolution_status = models.CharField(…, default="RESOLVED")

In practice, newly-created CorpusReference rows for law citations start as STATUS_EXTERNAL, and document references start as STATUS_UNRESOLVED until matched. The writer always sets this field explicitly so the default is never relied upon, but "RESOLVED" is the least-likely default for a new row and could cause confusion if the field is ever omitted in a write. STATUS_UNRESOLVED would be a safer default.

Minor / Style

9. auto_create_doc_analyzers not renamed (analyzer/utils.py)

The function now creates both doc-scoped and corpus-scoped analyzer rows, but its name is still auto_create_doc_analyzers. Consider renaming to auto_create_analyzers for clarity — the function was renamed in the analyzer checks.py comment but not the function itself.

10. Corpus node visibility consistency in GovernanceGraphService.build() (governance_graph_service.py)

Source documents from ref_rows are explicitly checked against visible_doc_ids. Documents from rel_rows (via DocumentRelationshipService.get_visible_relationships) are added to doc_corpus without going through that same visible_doc_ids check. The final Document.objects.filter(id__in=node_doc_ids) does not re-filter for visibility. This is presumably safe because both service calls already enforce visibility, but the defence-in-depth is uneven. A consistent visible_doc_ids guard for all node sources (or a single visibility-scoped final document query) would be cleaner.

11. Demo data size

The six demo/authority_seeds/*.json files (DGCL, Securities Act, Exchange Act, ICA, IRC, SEC Rules) account for a large portion of the +13,366 line count. They're fine for documentation, but these should be excluded from CI test runs that don't need them. The existing docs/test_scripts/ pattern handles this correctly.

Tests

Overall the test coverage is strong. A few gaps worth addressing:

• Missing: malformed relay ID on corpusReferences query — tests resolve_corpus_references with a non-relay corpus_id to confirm the fix for bug Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1.
• Missing: invalid types value — EnrichmentService.scan(types=["INVALID_TYPE"]) silently returns no candidates, which could confuse callers. Either add validation in the service or add a test that documents the silent-ignore behaviour.
• Missing: concurrent mention creation — _get_or_create_mention() uses filter().first() + save(), which is not atomic. Under concurrent runs (two apply() calls on the same corpus), the UniqueConstraint on CorpusReference protects the reference row, but the mention Annotation has no uniqueness constraint — concurrent runs could create duplicate annotations. The dedup comment acknowledges this but there's no test for the race.

Summary

The architecture is sound and the service-layer conventions are followed correctly throughout. The most important things to fix before merge are the malformed-ID 500 bug in the GraphQL resolver and the Analysis lifecycle inconsistency when running via the agent tool. The N+1 queries are a real concern at demo scale (1,943 references) and worth addressing before this ships to production.

Codecov Report

❌ Patch coverage is 93.41737% with 47 lines in your changes missing coverage. Please review.

📢 Thoughts on this report? Let us know!

Code Review — PR #1976: Corpus Reference Enrichment

This is a substantial, well-architected addition. The security posture is strong, the service layer is consistently used, and the test coverage is good for a first pass. A handful of findings below — two bugs and one N+1 are worth addressing before merge; the rest are follow-up quality work.

Security — No blocking issues

• Service layer enforced: GraphQL resolvers route through CorpusReferenceService / GovernanceGraphService / EnrichmentService with no inline visible_to_user fusions. The config/graphql/ E001 check passes.
• Ghost-node degradation: GovernanceGraphService correctly degrades invisible targets to ghost nodes (title/slug stripped, only relay-ID returned) — no existence oracle leak.
• Corpus decorator gate: @corpus_analyzer_task checks corpus visibility against the analysis creator via BaseService.filter_visible before the task body runs. The uniform error message (same whether corpus is missing or invisible) is correct.
• IDOR prevention: Annotations and references are always scoped by (document_id, corpus) — consistent with the project pattern.

Bugs

1. Regex quote-character class — extractor.py

The defined-term patterns comment says "curly or straight quotes" but the character class [\""] only matches a straight double-quote. Curly-quoted definitions like (the "Company") — common in SEC filings — will be silently skipped. Fix: include the Unicode left-curly (") and right-curly (") quote characters alongside the straight quote in both the opening-quote matcher and the negated inner group.

2. Section heading backward-search fallback — resolver.py

idx = doc_text.lower().find(heading.lower(), cand.end)
if idx == -1:
    idx = doc_text.lower().find(heading.lower())   # searches from position 0

When the heading appears multiple times (e.g., "Section 2. Definitions" in both an exhibit and the main agreement) and the first occurrence is before the reference span, the fallback matches the wrong instance. The resolved target_offset will point to an earlier, unrelated section. Consider skipping the fallback when the forward search misses, and leaving the resolution UNRESOLVED instead.

Moderate concerns

3. N+1 queries in the CorpusReference GraphQL resolver — annotation_queries.py

resolve_corpus_references() returns a bare queryset without select_related. If CorpusReferenceType accesses any FK (e.g. source_annotation, corpus, target_document) it will fire one extra query per row. Fix:

return CorpusReferenceService.for_corpus(user, corpus_id).select_related(
    "source_annotation", "corpus", "target_document", "target_annotation"
)

4. authority_alias_registry(user=None) leaks all authority metadata — authorities.py

qs = (
    Document.objects.all() if user is None else Document.objects.visible_to_user(user)
)

Any call without a user (e.g. a future Celery task that forgets to pass creator) silently returns aliases from ALL documents, including those in private corpuses. Not exploitable through current API paths, but it is a footgun for future callers. Recommend defaulting to an empty queryset when user is None, or requiring the caller to pass a user explicitly.

5. Redundant Analysis.objects.get() in task body — corpus_analysis_tasks.py

@corpus_analyzer_task already fetches and validates the Analysis before calling the function body, but the body does a second Analysis.objects.get(id=analysis_id) independently — the decorator's instance is not threaded through. Minor wasted query today; divergence risk if the decorator is extended. Consider passing the pre-fetched analysis as a kwarg, similar to how doc_analyzer_task handles the document.

Minor / polish

6. Overly broad except Exception — authorities.py

except Exception:  # unreadable -> rewrite below
    current = None

The intent appears to be handling "text is not a string", which should be except (TypeError, AttributeError): at most. Catching all exceptions would silently swallow unexpected errors and make them hard to debug.

7. Hardcoded trailing-punctuation strip — extractor.py

The rstrip(",.;:") literal appears inline rather than as a named constant. Since constants.py is already the home for grammar config, consider placing it there as TRAILING_PUNCT = ",.;:".

8. Dispatch gap worth a comment — corpus_tasks.py

The PR description flags "CorpusAction auto-bootstrap" as a future step, but get_corpus_analyzer_task_by_name is new and the dispatch path is only partially wired. A brief TODO comment (with an issue reference) would prevent this from being forgotten.

Test coverage gaps

• Permission denial: Tests exercise the happy path (corpus owner enriches their own corpus). There are no tests for a user trying to enrich a corpus they can read but not write, or one they cannot see. The decorator's visibility gate is new code and warrants a test.
• Concurrent enrichment: Two tasks running simultaneously on the same corpus both attempt get_or_create on the same (source_annotation, reference_type, canonical_key) tuples under separate transactions. The unique constraint on CorpusReference serializes those correctly, but the Annotation creation path deduplicates by filter().first(), not by a DB constraint — a race there could create duplicates. Worth a comment or test.
• Curly-quote extraction: Once the regex is fixed (finding Bump postgres from 14.5 to 15.0 in /compose/production/postgres #1), add a test case with curly-quote defined-term patterns.

Migration notes

Migration 0078 is a clean CreateModel. The indexes on (corpus, reference_type) and canonical_key are appropriate. Minor note: canonical_key is nullable with db_index=True — Postgres includes NULLs in the B-tree index. For large authority corpora a partial index (condition=Q(canonical_key__isnull=False)) would be more efficient, but not a blocker.

Summary

The architecture is solid, security practices are consistent with the rest of the codebase, and the @corpus_analyzer_task decorator is a clean abstraction.